[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign neolit123 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Hi @AmitSahastra. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-test

LGTM. Running this in my dev environment. So far no further issues.

@AmitSahastra bit curious about the use of EKSConfig here. Right now I've got about a hundred plus clusters on CAPA that are using AWSManagedMachinePools but I'm not using an EKSConfig with them. I guess my question is - will I have to plug in an EKSConfig for every cluster, in order to make the switch to AL2023? Also kind of curious as to how that might impact existing managed nodegroups running off AL2.

Really appreciate your work on this, looking forward to having it out soon!

@AmitSahastra bit curious about the use of EKSConfig here. Right now I've got about a hundred plus clusters on CAPA that are using AWSManagedMachinePools but I'm not using an EKSConfig with them. I guess my question is - will I have to plug in an EKSConfig for every cluster, in order to make the switch to AL2023? Also kind of curious as to how that might impact existing managed nodegroups running off AL2.

Really appreciate your work on this, looking forward to having it out soon!

Had you enable launch template in AWSMMP it should create EKS config.

Aye we are using launch templates in our managed machine pools using the AWSLaunchTemplate parameter. Pretty certain no corresponding EKSConfig objects are being created - because I don't see any on our management cluster - hence my question around whether one needs to create EKSConfig objects for each cluster.

Yes, for AL2023 support via CAPA, I added logic that uses nodeType: al2023 inside EKSConfig to generate the correct nodeadm-style userdata. So if you’re planning to switch to AL2023 and rely on CAPA for bootstrap, then yes, you’ll need to start creating EKSConfig objects with that field set.

As for existing AL2-based nodegroups, there’s no impact — they can stay as-is with your current launch templates and don’t require any changes unless you explicitly migrate them to AL2023.

I hope that answers your question.

That's baller - thanks a ton @AmitSahastra, appreciate you!

I have built and tested this PR on a sandbox environment for our use case.
Maybe it works for AWSManagedMachinePool, but it does not support AWSManagedControlPlane with MachineDeployment and EKSConfigTemplate.

It still creates instances with previous format and so, fails to connect.

---
apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: AWSMachineTemplate
metadata:
  name: test-3723977939
  namespace: flux-system
spec:
  template:
    spec:
      ami:
        eksLookupType: AmazonLinux2023
      instanceMetadataOptions:
        httpTokens: required
        httpPutResponseHopLimit: 2
      iamInstanceProfile: test-capa-nodes
      instanceType: t4g.medium
---
apiVersion: bootstrap.cluster.x-k8s.io/v1beta2
kind: EKSConfigTemplate
metadata:
  name: test-al2023
  namespace: flux-system
spec:
  template:
    spec:
      nodeType: al2023
---
apiVersion: cluster.x-k8s.io/v1beta1
kind: MachineDeployment
metadata:
  name: t00-use1-eks-test-us-east-1a-md0
  namespace: flux-system
spec:
  clusterName: test
  template:
    spec:
      bootstrap:
        configRef:
          apiVersion: bootstrap.cluster.x-k8s.io/v1beta2
          kind: EKSConfigTemplate
          name: test-al2023
      clusterName: test
      infrastructureRef:
        apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
        kind: AWSMachineTemplate
        name: test-3723977939
      version: v1.32.3

I0804 13:55:29.740725       1 eksconfig_controller.go:265] "Generating userdata" 
I0804 13:55:29.740740       1 eksconfig_controller.go:314] "Processing AL2023 node type" 
I0804 13:55:29.741301       1 eksconfig_controller.go:366] "Generating AL2023 userdata" 
I0804 13:55:29.748003       1 eksconfig_controller.go:443] "created bootstrap data secret for EKSConfig" 
I0804 13:55:29.766772       1 eksconfig_controller.go:197] "joinWorker called" [repeated multiple times, without success]

I0804 13:55:30.090681       1 awsmachine_controller.go:732] "Creating EC2 instance"
I0804 13:55:30.306418       1 instances.go:135] "Obtained a list of supported architectures for instance type"  [...] supported architectures=["arm64"]
I0804 13:55:30.307461       1 instances.go:135] "Chosen architecture" [...]  instance type="t4g.medium" supported architectures=["arm64"] architecture="arm64"
I0804 13:55:30.353337       1 ami.go:374] "found AMI" [...] id="ami-0cbd69473baf7c079" version="1.32"
I0804 13:55:32.550263       1 awsmachine_controller.go:578] "EC2 instance state changed" state="pending" instance-id="i-xxx
I0804 13:56:03.485209       1 awsmachine_controller.go:578] "EC2 instance state changed" state="running" instance-id="i-xxx"

After further investigation, I can be more precise. The secret is created as expected, with the expected content.
The vm is deployed with userdata created by pkg/cloud/services/secretsmanager/secret.go (template is in pkg/cloud/services/secretsmanager/secret_fetch_script.go). So far, my guess is that it's supposed to get the secret from SecretManager, and that's where it's failing.

EDIT: It works when disabling Secrets Manager on user metadata in AWSMachineTemplate:

@@ -11,6 +11,8 @@
       {{- else }}
         eksLookupType: AmazonLinux2023
       {{- end }}
+      cloudInit:
+        insecureSkipSecretsManager: true
       instanceMetadataOptions:
         httpTokens: required
         httpPutResponseHopLimit: 2

@AmitSahastra I've done a review with a commit on my fork https://github.com/mloiseleur/cluster-api-provider-aws/tree/al2023-launch-template

What I've done:

1. Fix maxPods (it was setting to 58 when setting the boolean in the CRD to true)
2. Make clusterDNS optional (following this comment, confirmed with my test that it works as expected)
3. Move node-labels from gotemplate to go func, for readability
4. Add usage documentation in the book
5. Tested everything on a sandbox env

May I invite you to add it to your PR ? You can apply this file or cherrypick from my branch.

Thanks @mloiseleur for the detailed review and improvements! I’ve cherry-picked your changes on maxPods, clusterDNS, and the node-labels refactor, and added them to the PR. Also appreciated the test validation and doc additions. 🙌

@AmitSahastra: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.